Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove non promicro pins from converters #18239

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Aug 31, 2022

Description

Seems these have incorrectly slipped through.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@zvecr zvecr added the bug label Aug 31, 2022
@zvecr zvecr requested a review from a team August 31, 2022 22:17
@github-actions github-actions bot added the core label Aug 31, 2022
Copy link
Member

@daskygit daskygit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming we're happy to be merging to master?

@drashna drashna merged commit 7adef85 into qmk:master Sep 1, 2022
@drashna
Copy link
Member

drashna commented Sep 1, 2022

I'm assuming we're happy to be merging to master?

I would say that it definitely qualifies as a bug fix, so yeah, it should.

@sadekbaroudi
Copy link

sadekbaroudi commented Sep 7, 2022

@drashna @megamind4089 @zvecr

Why was this removed? Almost every keyboard I have using the stemcell uses these pins. What is the alternative to utilize these GPIO when using a stemcell?

Edit: Note that I have 100 users or so who use these keyboards with a stemcell as well.

@sadekbaroudi sadekbaroudi mentioned this pull request Sep 7, 2022
14 tasks
@sadekbaroudi
Copy link

see #18236 (comment) - I have the same issue with that as this one...

@zvecr
Copy link
Member Author

zvecr commented Sep 7, 2022

Almost every keyboard I have using the stemcell uses these pins

Then they are not "pro micro compatible" and your issue extends out from the currently implemented converter targets.

@sadekbaroudi
Copy link

sadekbaroudi commented Sep 7, 2022

Then they are not "pro micro compatible" and your issue extends out from the currently implemented converter targets.

@zvecr - This doesn't address my point in the other PR ( 18236 )...

The architecture of "from pro micro" (which is implicit at this point, since the build syntax doesn't support a from parameter) doesn't make sense. You know the target hardware, you know the pinout. Your refusal to support the bottom pins is based on the underlying (edit: qmk) architecture, which is irrelevant for the consumer. They have X number of pins, they want to use them. Why limit it?

@zvecr
Copy link
Member Author

zvecr commented Sep 7, 2022

But it is relevant to the consumer. How would a user use the converter feature to put a kb2040 into something that expects an elite c and use of its bottom row of pins? We know the pinout in both cases, but they are physically incompatible. The interface advertised, ensures that these scenarios fail for a predictable reason. The reason for this PR was to address the incorrect assumptions that unfortunately include your use case, before they start to duplicate everywhere and become a maintenance issue.

As mentioned in the other ticket, it does support a "from". The default is just currently "promicro" to maintain existing compatibility. Adding in an elite_c -> stemcell converter and updating PIN_COMPATIBLE for affected boards would be the way forward. The intention is that longer term, keyboards define what dev board they use. That way we can enforce someone trying to use converters on something like a dz60.

Outside of converters, there is no limit to pins. Some choose to add "first class" support as a keyboard sub revision to directly control items like low level driver config.

@zvecr zvecr mentioned this pull request Sep 7, 2022
14 tasks
@sadekbaroudi
Copy link

This translated into a discord conversation. For future reference (if needed):
https://discord.com/channels/440868230475677696/1017176320318373939/1017178935668265061

ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants